Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Splitstore Review #6718

Closed
wants to merge 1 commit into from
Closed

Splitstore Review #6718

wants to merge 1 commit into from

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented Jul 9, 2021

Reviewing #6474

Unfortunately, github makes it difficult to review rewrites because the patches are nasty. So I'm just adding comments to the code directly.

To respond, please review this PR. I'll continue to push new comments inline.

@Stebalien Stebalien changed the title initial review comments Splitstore Review Jul 9, 2021
@Stebalien Stebalien requested a review from vyzo July 9, 2021 07:08
@Stebalien Stebalien marked this pull request as draft July 9, 2021 07:09
blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
@Stebalien Stebalien force-pushed the steb/review/splitstore-redux branch from 5ed35cd to 723cfee Compare July 10, 2021 05:31
@Stebalien
Copy link
Member Author

Have we tried exercising this code by syncing the chain? I assume most of our "realistic" tests have been one offs so it would be nice to have a test that rapidly and repeatedly purges.

@vyzo
Copy link
Contributor

vyzo commented Jul 10, 2021

Have we tried exercising this code by syncing the chain? I assume most of our "realistic" tests have been one offs so it would be nice to have a test that rapidly and repeatedly purges.

Yes, I have two live nodes running latest code for about a week now, one with discard and the other with a coldstore. Both are syncing fine and exhibit no ill effects, with block validation time unaffected by a running compaction.

However, as we've been discussing with @raulk, we do need a harness test so that we can reliably and reproducibly do automated testing. In my opinion, this is critical and necessary before we declare the code production ready. I will create follow up issue as it is unlikely we'll get to it before code freeze for v1.11.1.

blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed review comments in 0c5e336

And fixed a bug a lint complaint made me see in df9670c

blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
blockstore/splitstore/splitstore.go Outdated Show resolved Hide resolved
@vyzo
Copy link
Contributor

vyzo commented Jul 10, 2021

Created follow up issue for testing in #6725
It is not urgent, but we definitely need to resolve this in the next few months.

@Stebalien Stebalien closed this Jul 13, 2021
@Stebalien Stebalien force-pushed the steb/review/splitstore-redux branch from 723cfee to 60212c8 Compare July 13, 2021 04:41
@Stebalien Stebalien reopened this Jul 13, 2021
@Stebalien Stebalien changed the base branch from feat/splitstore-redux to master July 13, 2021 05:03
@Stebalien Stebalien changed the base branch from master to feat/splitstore-redux July 13, 2021 05:04
@Stebalien
Copy link
Member Author

Ok, final final review.

Base automatically changed from feat/splitstore-redux to master July 13, 2021 10:43
@jacobheun
Copy link
Contributor

Closing this as #6474 was merged.

@jacobheun jacobheun closed this Jul 14, 2021
@Stebalien Stebalien deleted the steb/review/splitstore-redux branch August 25, 2021 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants